Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable reload-interval flag for all TLS server configs #6525

Merged
merged 9 commits into from
Jan 12, 2025
Merged

Enable reload-interval flag for all TLS server configs #6525

merged 9 commits into from
Jan 12, 2025
+16 −66

Conversation

ADI-ROXX
Copy link
Contributor

@ADI-ROXX ADI-ROXX commented Jan 11, 2025 •
edited
Loading

Which problem is this PR solving?

Description of the changes

  • Added the field EnableCertReloadInterval: true everywhere ServerFlagsConfig is used
  • As a dual check for future use, set the value of c.EnableCertReloadInterval to true in AddFlags function in pkg/config/tlscfg/flags.go

How was this change tested?

Checklist

@ADI-ROXX ADI-ROXX requested a review from a team as a code owner January 11, 2025 14:15
@ADI-ROXX ADI-ROXX requested a review from albertteoh January 11, 2025 14:15
@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Jan 11, 2025 •
edited
Loading

@yurishkuro Some tests are failing because of the changes that are made. Most probably it is because the tests defined are not expecting reload-interval to be true.

So to solve that, I think the way ahead is to make changes in the tests that were failing

@ADI-ROXX
Removed property
99809eb 
Signed-off-by: cs-308-2023 <[email protected]>
@ADI-ROXX
Copy link
Contributor Author

@yurishkuro I have removed the EnableCertReloadInterval from everywhere. Can you check now. Some test cases are failing though since they are made for EnableCertReloadInterval=false.

mahadzaryab1
mahadzaryab1 reviewed Jan 11, 2025
View reviewed changes
pkg/config/tlscfg/flags.go Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pkg/config/tlscfg/flags_test.go Outdated Show resolved Hide resolved
pkg/config/tlscfg/flags_test.go Outdated Show resolved Hide resolved
pkg/config/tlscfg/flags_test.go
@@ -131,13 +131,11 @@ func TestServerCertReloadInterval(t *testing.T) {
{
config: ServerFlagsConfig{
Prefix: "enabled",
EnableCertReloadInterval: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test anymore. We can just move the parsing of this flag to TestServerFlags

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So shall I remove this test completely?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ADI-ROXX Yeah - I think we can remove this test but we don't want to lose coverage of this flag. So we can add this flag as part of the test setup in TestServerFlags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can add this flag as part of the test setup in TestServerFlags

I am not able to understand what is meant by this. What do I have to add in TestServerFlags? Because EnableCertReloadInterval is no longer in ServerFlagsConfig, what is to be done now?

@ADI-ROXX
Removed comments
5edf8f0 
Signed-off-by: cs-308-2023 <[email protected]>
@ADI-ROXX
Removed comma
cd01487 
Signed-off-by: cs-308-2023 <[email protected]>
yurishkuro
yurishkuro reviewed Jan 11, 2025
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
@ADI-ROXX ADI-ROXX changed the title [WIP] Added the field of EnableCertReloadInterval: true everywhere it is used and in the function Remove the property EnableCertReloadInterval to always allow reload-interval for TLS config Jan 11, 2025
@ADI-ROXX ADI-ROXX changed the title Remove the property EnableCertReloadInterval to always allow reload-interval for TLS config Remove the property EnableCertReloadInterval to always allow reload-interval Jan 11, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 11, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (89c4ee4) to head (e1d7fe2).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #6525       +/-   ##
===========================================
+ Coverage   50.21%   96.25%   +46.03%     
===========================================
  Files         188      372      +184     
  Lines       11405    21276     +9871     
===========================================
+ Hits         5727    20479    +14752     
+ Misses       5220      609     -4611     
+ Partials      458      188      -270
Flag Coverage Δ
badger_v1 10.68% <0.00%> (+<0.01%) ⬆️
badger_v2 2.78% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v1-manual 16.58% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v2-auto 2.71% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v2-manual 2.71% <0.00%> (+<0.01%) ⬆️
cassandra-5.x-v1-manual 16.58% <0.00%> (+<0.01%) ⬆️
cassandra-5.x-v2-auto 2.71% <0.00%> (+<0.01%) ⬆️
cassandra-5.x-v2-manual 2.71% <0.00%> (+<0.01%) ⬆️
elasticsearch-6.x-v1 20.25% <0.00%> (-0.01%) ⬇️
elasticsearch-7.x-v1 20.33% <0.00%> (+<0.01%) ⬆️
elasticsearch-8.x-v1 20.49% <0.00%> (+<0.01%) ⬆️
elasticsearch-8.x-v2 2.77% <0.00%> (+<0.01%) ⬆️
grpc_v1 12.20% <0.00%> (+<0.01%) ⬆️
grpc_v2 9.03% <0.00%> (-0.01%) ⬇️
kafka-3.x-v1 10.36% <0.00%> (+<0.01%) ⬆️
kafka-3.x-v2 2.78% <0.00%> (+<0.01%) ⬆️
memory_v2 2.78% <0.00%> (+<0.01%) ⬆️
opensearch-1.x-v1 20.37% <0.00%> (-0.01%) ⬇️
opensearch-2.x-v1 20.37% <0.00%> (-0.01%) ⬇️
opensearch-2.x-v2 2.77% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.51% <0.00%> (+<0.01%) ⬆️
unittests 95.13% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ADI-ROXX ADI-ROXX changed the title Remove the property EnableCertReloadInterval to always allow reload-interval Include reload-interval for each tls config Jan 11, 2025
@yurishkuro
fix
c0e8dce 
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro
add-test
a44a401 
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro changed the title Include reload-interval for each tls config Enable reload-interval flag for all TLS server configs Jan 12, 2025
@yurishkuro yurishkuro enabled auto-merge (squash) January 12, 2025 00:39
@yurishkuro yurishkuro disabled auto-merge January 12, 2025 00:47
@yurishkuro yurishkuro merged commit c7e0b78 into jaegertracing:main Jan 12, 2025
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahadzaryab1 mahadzaryab1 mahadzaryab1 left review comments

@yurishkuro yurishkuro yurishkuro approved these changes

@albertteoh albertteoh Awaiting requested review from albertteoh albertteoh is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
changelog:bugfix-or-minor-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Always include reload-interval option for TLS config
3 participants
@ADI-ROXX @yurishkuro @mahadzaryab1